Skip to content

Comments

CORS-2728: Nutanix failure domains support#1578

Merged
openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom
nutanix-cloud-native:ocp-multi-pe
Nov 20, 2023
Merged

CORS-2728: Nutanix failure domains support#1578
openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom
nutanix-cloud-native:ocp-multi-pe

Conversation

@yanhua121
Copy link
Contributor

@yanhua121 yanhua121 commented Sep 5, 2023

https://issues.redhat.com/browse/OCPSTRAT-257

  • Adding NutanixFailureDomain struct
  • Adding the optional field FailureDomains to the Infrastructure NutanixPlatformSpec struct
  • Adding the optional field Nutanix to the ControlPlaneMachineSet FailureDomains struct
  • Adding the optional field FailureDomainName to the NutanixMachineProviderConfig struct

The proof of concept PRs using the Nutanix failure domains api:
openshift/cluster-control-plane-machine-set-operator#258
openshift/machine-api-provider-nutanix#56

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 5, 2023

Hello @yanhua121! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@openshift-ci openshift-ci bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 5, 2023
@openshift-ci openshift-ci bot requested review from eparis and mandre September 5, 2023 16:08
@yanhua121
Copy link
Contributor Author

/assign @JoelSpeed

@yanhua121
Copy link
Contributor Author

/retest-required

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have proof of concepts for how this will work with Machines and then ControlPlaneMachineSets? I'm curious as to whether we really need/want to put this failure domain information into the infrastructure object, or just follow the patterns set out by AWS/Azure/GCP for this.

That said, vSphere has embarked on a similar route to this, so perhaps this is a good path forward.

The open questions I have currently are:

  • How will this work for Machines? How will they be configured and validated
  • How will this work in a cluster that is upgraded?
  • How will the CPMS injection the failure domain into the providerSpec/know which machines to update?
  • Are the failure domains mutable? If so, how can you tell that a machine is correctly installed within a failure domain?

@yanhua121
Copy link
Contributor Author

Do we have proof of concepts for how this will work with Machines and then ControlPlaneMachineSets? I'm curious as to whether we really need/want to put this failure domain information into the infrastructure object, or just follow the patterns set out by AWS/Azure/GCP for this.

That said, vSphere has embarked on a similar route to this, so perhaps this is a good path forward.

The open questions I have currently are:

  • How will this work for Machines? How will they be configured and validated
  • How will this work in a cluster that is upgraded?
  • How will the CPMS injection the failure domain into the providerSpec/know which machines to update?
  • Are the failure domains mutable? If so, how can you tell that a machine is correctly installed within a failure domain?

@JoelSpeed See the feature design doc at https://docs.google.com/document/d/1TA9vCH-3X_GttJ4fHg39sstdhBPCR7S83N1R8rzXFuk/.
We can schedule a meeting to discuss these questions.

@JoelSpeed
Copy link
Contributor

@yanhua121 If you have work in progress implementations for the CPMS and Machine related components could you please open the PRs as WIP so that we can review how this code is going to be used in the future

@yanhua121
Copy link
Contributor Author

@yanhua121 If you have work in progress implementations for the CPMS and Machine related components could you please open the PRs as WIP so that we can review how this code is going to be used in the future

@JoelSpeed Okay. Can you take another look at this PR, I made changes based on your review comments and our discussion.

@yanhua121
Copy link
Contributor Author

/retest-required

@yanhua121
Copy link
Contributor Author

/assign @elmiko @rvanderp3

@yanhua121 yanhua121 force-pushed the ocp-multi-pe branch 3 times, most recently from 44627b5 to 8644753 Compare November 17, 2023 18:05
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, yanhua121

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 20, 2023

@yanhua121: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit b867610 into openshift:master Nov 20, 2023
@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-config-api-container-v4.15.0-202311210610.p0.gb867610.assembly.stream for distgit ose-cluster-config-api.
All builds following this will include this PR.

@yanhua121 yanhua121 changed the title Nutanix failure domains support CORS-2728: Nutanix failure domains support Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants